feat(secretmanager): Adding tags samples#13745
feat(secretmanager): Adding tags samples#13745khilan-crest wants to merge 9 commits intoGoogleCloudPlatform:mainfrom
Conversation
product-auto-label
bot
added
api: secretmanager
Summary of ChangesHello @khilan-crest, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request expands the Google Cloud Secret Manager Python samples by adding new code examples for managing resource tags. Specifically, it provides utilities to detach existing tag bindings from secrets and to list all tags currently associated with a secret. These new samples cater to both global and regional secret configurations, offering developers clear guidance on how to programmatically interact with the tagging features of Secret Manager. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new samples for managing tags on secrets in Secret Manager, for both global and regional secrets. The changes include scripts for listing and detaching tag bindings, along with corresponding tests. My review focuses on improving code clarity, correctness of documentation, and consistency in testing. I've suggested refactoring some loops to be more idiomatic, correcting misleading docstrings, and enhancing a test case to be more robust.
| location_id (str): Region where the secret should be stored (e.g., "us-central1") | ||
| secret_id (str): ID of the secret to create | ||
|
|
||
| Example: | ||
| # Create a regional secret with a customer-managed encryption key | ||
| list_regional_secret_tag_bindings( | ||
| "my-project", | ||
| "my-regional-secret-with-cmek", | ||
| "us-central1", | ||
| ) |
There was a problem hiding this comment.
The docstring contains several inaccuracies that could confuse users:
- The description for
location_idimplies creation (should be stored), but the function is for listing. - The description for
secret_idsaysID of the secret to create, which is incorrect. - The example comment is misleadingly about creating a secret.
- The arguments in the example call for
location_idandsecret_idare swapped.
This suggestion corrects these issues.
| location_id (str): Region where the secret should be stored (e.g., "us-central1") | |
| secret_id (str): ID of the secret to create | |
| Example: | |
| # Create a regional secret with a customer-managed encryption key | |
| list_regional_secret_tag_bindings( | |
| "my-project", | |
| "my-regional-secret-with-cmek", | |
| "us-central1", | |
| ) | |
| location_id (str): Region where the secret is stored (e.g., "us-central1") | |
| secret_id (str): ID of the secret to list tag bindings for | |
| Example: | |
| # List tag bindings for a regional secret | |
| list_regional_secret_tag_bindings( | |
| "my-project", | |
| "us-central1", | |
| "my-regional-secret", | |
| ) |
| parent = f"//secretmanager.googleapis.com/{secret_name}" | ||
|
|
||
| # List all tag bindings. | ||
| tag_bindings = [] |
There was a problem hiding this comment.
The tag_bindings list is initialized but its value is never used. It can be removed to simplify the code.
| print(f"Tag bindings for {secret_name}:") | ||
| for binding in bindings: | ||
| print(f"- Tag Value: {binding.tag_value}") | ||
| tag_bindings.append(binding) |
There was a problem hiding this comment.
Since the tag_bindings list is not used, this append operation is unnecessary and can be removed.
secretmanager/snippets/regional_samples/list_regional_secret_tag_bindings.py
Show resolved
Hide resolved
| def test_detach_tag( | ||
| project_id: str, | ||
| tag_key_and_tag_value: Tuple[str, str], | ||
| secret_id: str, | ||
| ) -> None: | ||
| """Test detaching a tag from a secret.""" | ||
| # Get the tag value from the fixture | ||
|
|
||
| _, tag_value = tag_key_and_tag_value | ||
|
|
||
| # First bind the tag to the secret | ||
|
|
||
| bind_tags_to_secret(project_id, secret_id, tag_value) | ||
| secret_name = f"projects/{project_id}/secrets/{secret_id}" | ||
|
|
||
| # Now detach the tag | ||
|
|
||
| detach_tag(project_id, secret_id, tag_value) | ||
|
|
||
| client = resourcemanager_v3.TagBindingsClient() | ||
| parent = f"//secretmanager.googleapis.com/{secret_name}" | ||
| request = resourcemanager_v3.ListTagBindingsRequest(parent=parent) | ||
|
|
||
| # Check that none of the bindings contain our tag value | ||
|
|
||
| tag_found = False | ||
| for binding in client.list_tag_bindings(request=request): | ||
| if binding.tag_value == tag_value: | ||
| tag_found = True | ||
| break | ||
| assert ( | ||
| not tag_found | ||
| ), f"Tag value {tag_value} should have been detached but was found" |
There was a problem hiding this comment.
For consistency with test_detach_regional_tag, this test should also verify the output message from the detach_tag function. This requires adding the capsys fixture to the test function and asserting that the success message is present in the output.
def test_detach_tag(
capsys: pytest.LogCaptureFixture,
project_id: str,
tag_key_and_tag_value: Tuple[str, str],
secret_id: str,
) -> None:
"""Test detaching a tag from a secret."""
# Get the tag value from the fixture
_, tag_value = tag_key_and_tag_value
# First bind the tag to the secret
bind_tags_to_secret(project_id, secret_id, tag_value)
secret_name = f"projects/{project_id}/secrets/{secret_id}"
# Now detach the tag
detach_tag(project_id, secret_id, tag_value)
out, _ = capsys.readouterr()
assert "Detached tag value" in out
client = resourcemanager_v3.TagBindingsClient()
parent = f"//secretmanager.googleapis.com/{secret_name}"
request = resourcemanager_v3.ListTagBindingsRequest(parent=parent)
# Check that none of the bindings contain our tag value
tag_found = False
for binding in client.list_tag_bindings(request=request):
if binding.tag_value == tag_value:
tag_found = True
break
assert (
not tag_found
), f"Tag value {tag_value} should have been detached but was found"
|
Here is the summary of changes. You are about to add 4 region tags.
This comment is generated by snippet-bot.
|
1 participant
Adding tags samples
Fixes #
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
nox -s py-3.9(see Test Environment Setup)nox -s lint(see Test Environment Setup)